Conversation
Keep externref global imports as externrefs at the module boundary and insert their values into the externref table during instantiation via an active element segment. Replace the original globals with internal shared i31 globals initialized to their corresponding table indices. This keeps e.g. imported string constants working as expected without assuming that they can be converted to `(ref (shared extern))`, which would not work if they flowed to the same locations as other externrefs, which would have been lowered to i31ref instead. Now that both the function and externref tables have initial content, move the logic for creating the element segment to LazyTable.
aheejin
left a comment
aheejin
left a comment
There was a problem hiding this comment.
Mostly LGTM to me, modulo nits.
A corner case Gemini found. Do we handle this?
(import "env" "g" (global $g externref))
(export "out" (global $g))| WalkerPass::doWalkModule(wasm); | ||
| } | ||
|
|
||
| struct ImportToWrap { |
There was a problem hiding this comment.
| struct FuncImportToWrap { |
Now we have GlobalImportToWrap..
| rewriteTypes(); | ||
|
|
||
| for (auto& info : globalImportsToWrap) { | ||
| wrapGlobalImport(info.global, info.origType); |
There was a problem hiding this comment.
Nit: If we are to pass the two fields from GlobalImportToWrap, why not just pass the struct?
There was a problem hiding this comment.
You mean store Global instead of GlobalImportToWrap? That would require making copies of the original Globals because we need the original types here to survive type updating. It seems simpler to define a small helper struct rather than deal with setting unused values for mutable_, init, etc. on Globals.
| std::vector<GlobalImportToWrap> globalImportsToWrap; | ||
|
|
||
| void visitGlobal(Global* curr) { | ||
| if (curr->imported() && externTable.isTableType(curr->type)) { |
There was a problem hiding this comment.
Nit: It's preexisting, but isTableType sounds a little confusing to me, because it sounds like asking if the argument is a table type. How about canStore or something? (Not necessarily in this PR)
| } | ||
|
|
||
| addFunctionTable(); | ||
| funcTable.maybeAdd(funcTable.init.size()); |
There was a problem hiding this comment.
Nit: Keeping addFunctionTable (even though it's one-liner) feels more consistent that we also have addExternTable
| importGlobal->module = global->module; | ||
| importGlobal->base = global->base; | ||
| importGlobal->type = origType; | ||
| importGlobal->mutable_ = false; |
There was a problem hiding this comment.
Does this not support mutable imported global? If so, maybe worth adding a comment on why
There was a problem hiding this comment.
Correct. Supporting mutable globals would involve rewriting global.set to table.set, but if the mutable global is imported or exported, the surrounding JS would similarly have to be rewritten. I'll add a comment.
| ;; with i31 globals referring to their table indices. | ||
| ;; CHECK: (type $struct (shared (struct (field (ref null (shared i31))) (field (ref (shared i31)))))) | ||
| (type $struct (struct (field externref) (field (ref extern)))) | ||
| (import "env" "g_nullable" (global $g_nullable externref)) |
There was a problem hiding this comment.
What happens if JS passes a null for this? Does ref.is_null work? After the transformation, it's an i31ref now, which is not null
There was a problem hiding this comment.
For types that are transformed to i31ref, null values become (ref.null none) (i.e. a null i31ref).
This branch has not been deployed
2 participants
Keep externref global imports as externrefs at the module boundary and insert their values into the externref table during instantiation via an active element segment. Replace the original globals with internal shared i31 globals initialized to their corresponding table indices. This keeps e.g. imported string constants working as expected without assuming that they can be converted to
(ref (shared extern)), which would not work if they flowed to the same locations as other externrefs, which would have been lowered to i31ref instead.Now that both the function and externref tables have initial content, move the logic for creating the element segment to LazyTable.